Conversation
The specification only allows the server to return a null TTL, it doesn't allow the client to request that. This is intentional, as it forces clients to make a choice betwen a specific TTL or not caring what the TTL is, rather than defaulting to keeping tasks forever all the time. Allowing the requested value to be null was an implementation oversight that diverged from the spec.
…ript-sdk into chore/task-refactors-1
…ript-sdk into chore/task-refactors-1
🦋 Changeset detectedLatest commit: 61b07db The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correction: my previous red flag about schemas.ts not being modified was incorrect — the diff does include the ttl: z.number().optional() change, and the requestedTtl type narrowing in interfaces.ts is also present. Apologies for the noise. The PR looks well-structured, but since it introduces breaking changes to core schemas and interfaces across 8 files, I'd recommend a human reviewer confirm the design decisions.
Extended reasoning...
Overview
This PR makes two changes to the experimental task APIs: (1) disallows null TTL in TaskCreationParams (only valid in server responses per MCP spec), and (2) narrows the requestedTtl type from number | null to number in CreateTaskServerContext, TaskServerContext, and TaskContext. It touches 8 files: the Zod schema in schemas.ts, TypeScript interfaces in interfaces.ts and taskManager.ts, migration docs in migration.md and migration-SKILL.md, a changeset, and two test files.
Correction of prior review
My previous inline comment (ID 2994023669) incorrectly claimed that schemas.ts was not modified and that the new test would fail. This was wrong — the diff clearly shows ttl: z.union([z.number(), z.null()]).optional() changed to ttl: z.number().optional() in schemas.ts, and the requestedTtl types in interfaces.ts:29 and interfaces.ts:37 were properly narrowed from number | null to number. The two earlier comments (JSDoc inconsistency, _task vs task) were both resolved by the author.
Security risks
No security risks identified. The changes tighten validation (rejecting null where it was previously accepted), which is a strictness improvement. No auth, crypto, or permissions code is affected.
Level of scrutiny
This PR warrants human review despite being in experimental APIs. It introduces intentional breaking changes to core Zod schemas and TypeScript interfaces that affect the public API surface. The migration documentation additions should be reviewed for accuracy by someone familiar with the task API design. The changeset marks this as a patch bump for @modelcontextprotocol/client and @modelcontextprotocol/server, which may be appropriate for experimental APIs but a human should confirm the versioning decision.
Other factors
The PR includes good test coverage — new TaskCreationParams schema validation tests and updated InMemoryTaskStore tests that verify the behavioral change (omitted TTL instead of null TTL for unlimited lifetime). The code changes are internally consistent across schema, types, and tests. The JSDoc inconsistency noted in my first (resolved) comment about TaskSchema.ttl vs TaskCreationParamsSchema.ttl wording is minor and arguably correct since they describe different contexts (server response vs client request).
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
LGTM thanks for picking this up. Keeping it open for ~1-2 days to give @LucaButBoring a chance to take a look as well.
3 participants
Based on #1315 by @LucaButBoring , resolved conflicts
nullTTL in a task, as that isn't in the spec for requests (only for responses) - this was an oversight in the initial implementation.Motivation and Context
#1275
One topic from #1275 that doesn't have any changes here is the
?vs.undefinedthing - I took a look through the task interfaces and we use?consistently, which I think is the right approach. Let me know if there's any disagreement on this, and I can adjust things appropriately.How Has This Been Tested?
Adjusted unit tests for the grouping, and added some unit tests for disallowing
nullin requests.Breaking Changes
Both of these are breaking changes, and are therefore intended to be part of the v2 SDK. I think it would be reasonable to backport one or both commits to the v1 branch (especially given this is clearly marked as experimental), but I'm defaulting to v2 here.
Types of changes
Checklist
Additional context
Closes #1275